Fix func build/deploy validation: Add early validation for conflicting --image and --registry flags #3066

RayyanSeliya · 2025-10-04T18:02:24Z

Changes

🐛 Fix confusing UX when using conflicting --image and --registry flags
🧹 Implement 2-layer error system for image/registry validation
🎁 Add clear guidance for resolving flag conflicts

/kind enhancement

Fixes #3065

Solution:

Implement a 2-layer error system:

Layer 1 (Technical): Return ErrConflictingImageAndRegistry from validation in pkg/functions/errors.go
Layer 2 (CLI): Catch the error in cmd/build.go and cmd/deploy.go and wrap with user-friendly guidance

Implementation Details:

Add validation logic in buildConfig.Validate() and deployConfig.Validate() methods
Use strings.HasPrefix() to check if image starts with registry (allows subnamespaces)
Maintain backward compatibility with existing test cases
Ensure error messages are consistent between build and deploy commands

Release Note

func build and func deploy commands now validate conflicting --image and --registry flags before starting the build process, providing clear error messages and actionable guidance instead of misleading build failures.

knative-prow · 2025-10-04T18:02:34Z

Hi @RayyanSeliya. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2025-10-04T18:19:11Z

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.47%. Comparing base (c1b9a38) to head (0fb76e0).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/config_git_set.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3066      +/-   ##
==========================================
+ Coverage   58.66%   59.47%   +0.80%     
==========================================
  Files         133      133              
  Lines       17120    17153      +33     
==========================================
+ Hits        10044    10202     +158     
+ Misses       6146     5992     -154     
- Partials      930      959      +29

Flag	Coverage Δ
e2e-tests	`40.55% <2.50%> (+1.25%)`	⬆️
integration-tests	`54.18% <97.50%> (+1.74%)`	⬆️
unit-tests	`46.48% <97.50%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

RayyanSeliya · 2025-10-04T19:28:31Z

I think i am missing some edge cases @gauron99 @lkingland please point out if any the test are still failing :(

gauron99 · 2025-10-04T19:33:05Z

im gonna take a look tomorrow

RayyanSeliya · 2025-10-04T19:34:10Z

Sure no worries !! Good night !

gauron99

perhaps we could simply check that -registry and --image are not given together at all? Image takes precedence now which is correct. But we want to avoid having the need to remove FUNC_REGISTRY or your registry in config file when trying to once specify --image for a different registry.
In other words --image should just take precedence and work even when FUNC_REGISTRY is specified BUT --image and --registry could just return an error saying "just choose one or the other" for simplicity?

Otherwise you would need to check both registries, determine what parts image contains and prepend (which I dont think works currently? check my other comment) and that would require a bit more work and consideration.

@lkingland WDYT?

gauron99 · 2025-10-05T07:09:19Z

cmd/build.go

+		// Skip validation if image has digest (@sha256:...) - digests are immutable overrides
+		if !strings.Contains(c.Image, "@sha256:") {
+			// Only validate if image has explicit registry (contains "/" or ":")
+			// Simple names like "myimage" are compatible - registry will be prepended


are they really prepended?

❯ func build --image mine --registry quay.io/dfridric Building function image Still building 🙌 Function built: mine

@gauron99 u are right! The old validation was confusing , your test showed that --image override/takes precedence --registry, which wasn’t clear to users. We’ve fixed this by rejecting both flags when used together and showing a clear error message.

lkingland · 2025-10-06T02:14:46Z

I agree with @gauron99 on this one. Please simply check if the flags were both provided, and then raise an error that only one or the other should be provided, not both. 👍🏻

…between image and registry with different specified resgistry Signed-off-by: RayyanSeliya <[email protected]>

…ding both flags together with clear error message Signed-off-by: RayyanSeliya <[email protected]>

gauron99

nice, this already looks cleaner and simpler. I added a comment about one test

gauron99 · 2025-10-07T07:39:46Z

cmd/deploy_test.go

-			// resultant image member is updated but the registry member is not
-			// updated (subsequent builds will not be affected).
+			// Image flag sets the image directly when no registry is present
 			name: "image flag overrides",


I think in this test we want to test exactly the case where image DOES override registry since registry is not being provided via flag but rather its already been given previously.

This test seems to to to cover the case when you

func deploy --registry ...
... now f.registry exists in func.yaml and is not empty

func deploy --image <different image>

we want this case to work -> override the image from registry thats been used from 1. with image from 2.
the func.yaml registry is NOT overriden and kept as is from 1. but the new image is used in deployment because --image was provided with different value.

thx for the clarification @gauron99 i have updated to included this test too ...

Signed-off-by: RayyanSeliya <[email protected]>

lkingland

/lgtm
/hold to make sure @gauron99 your OK too

gauron99

/lgtm
/approve
/unhold

knative-prow · 2025-10-08T05:52:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauron99, lkingland, RayyanSeliya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gauron99,lkingland]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot added the kind/enhancement label Oct 4, 2025

knative-prow bot requested review from dsimansk and jrangelramos October 4, 2025 18:02

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2025

gauron99 reviewed Oct 5, 2025

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2025

RayyanSeliya added 3 commits October 6, 2025 23:26

validate first instead of building when conflicting flag combination …

8962344

…between image and registry with different specified resgistry Signed-off-by: RayyanSeliya <[email protected]>

refactor to include all the edge cases covered to not fail the ci test

0fa9976

validate conflicting --image and --registry flags prevent users provi…

002fd40

…ding both flags together with clear error message Signed-off-by: RayyanSeliya <[email protected]>

RayyanSeliya force-pushed the fix-flagcmbn-db branch from 73b89f5 to 002fd40 Compare October 7, 2025 07:26

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2025

gauron99 reviewed Oct 7, 2025

View reviewed changes

refactor based on the review

0fb76e0

Signed-off-by: RayyanSeliya <[email protected]>

RayyanSeliya requested a review from gauron99 October 7, 2025 11:57

lkingland approved these changes Oct 8, 2025

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2025

knative-prow bot assigned lkingland Oct 8, 2025

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2025

gauron99 approved these changes Oct 8, 2025

View reviewed changes

knative-prow bot assigned gauron99 Oct 8, 2025

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2025

knative-prow bot merged commit d9efd36 into knative:main Oct 8, 2025
39 checks passed

Fix func build/deploy validation: Add early validation for conflicting --image and --registry flags #3066

Fix func build/deploy validation: Add early validation for conflicting --image and --registry flags #3066

Uh oh!

Conversation

RayyanSeliya commented Oct 4, 2025

Changes

Solution:

Implementation Details:

Uh oh!

knative-prow bot commented Oct 4, 2025

Uh oh!

codecov bot commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RayyanSeliya commented Oct 4, 2025

Uh oh!

gauron99 commented Oct 4, 2025

Uh oh!

RayyanSeliya commented Oct 4, 2025

Uh oh!

gauron99 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

RayyanSeliya Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

lkingland commented Oct 6, 2025

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

RayyanSeliya Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

lkingland left a comment

Choose a reason for hiding this comment

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 4, 2025 •

edited

Loading

gauron99 left a comment •

edited

Loading